fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens#7167
fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens#7167diegolmello wants to merge 1 commit intofeat.voip-lib-newfrom
Conversation
… tokens Made-with: Cursor
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
169-181:⚠️ Potential issue | 🟠 Major
startCallByRoomleavesstartCall's promise unhandled.Now that
startCallreturnsPromise<void>(line 177), the fire-and-forget invocation at line 173 will surface any rejection (e.g.,this.instance?.startCallthrowing) as an unhandled promise rejection, and the original caller gets no signal of failure.CreateCall.tsxwas updated to await/try-catch, but this path was not.Attach at least a
.catch(or makestartCallByRoomasync and rethrow/log) so licensing/WebRTC failures from the room-initiated flow are observed.🐛 Proposed fix
- public startCallByRoom = (room: TSubscriptionModel | ISubscription) => { + public startCallByRoom = (room: TSubscriptionModel | ISubscription) => { useCallStore.getState().setRoomId(room.rid ?? null); const otherUserId = getUidDirectMessage(room); if (otherUserId) { - this.startCall(otherUserId, 'user'); + this.startCall(otherUserId, 'user').catch(error => { + console.error('[VoIP] Error starting call from room:', error); + }); } };As per coding guidelines: "Use explicit error handling with try/catch blocks for async operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 169 - 181, startCallByRoom currently calls the async startCall fire-and-forget which can produce unhandled rejections; update startCallByRoom to be async and await this.startCall(...) inside a try/catch (or at minimum attach a .catch) so errors from this.instance?.startCall are observed—use the same logger used elsewhere (e.g., console.error or the module's logger) to log the error and optionally rethrow or propagate a failure signal instead of silently dropping it; reference the startCallByRoom and startCall methods and the getUidDirectMessage call to locate where to add the await/try-catch.
🧹 Nitpick comments (2)
app/lib/methods/enterpriseModules.ts (1)
65-68: Extract'teams-voip'into a named constant for consistency.The other license identifiers in this file are declared as top-level constants (
LICENSE_OMNICHANNEL_MOBILE_ENTERPRISE,LICENSE_LIVECHAT_ENTERPRISE). The same pattern would apply here and remove the magic string duplicated withuseMediaCallPermission.tsanduseMediaCallPermissionusages.♻️ Proposed refactor
const LICENSE_OMNICHANNEL_MOBILE_ENTERPRISE = 'omnichannel-mobile-enterprise'; const LICENSE_LIVECHAT_ENTERPRISE = 'livechat-enterprise'; +const LICENSE_TEAMS_VOIP = 'teams-voip'; @@ export function isVoipModuleAvailable() { const { enterpriseModules } = reduxStore.getState(); - return enterpriseModules.includes('teams-voip'); + return enterpriseModules.includes(LICENSE_TEAMS_VOIP); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/enterpriseModules.ts` around lines 65 - 68, Extract the magic string 'teams-voip' into a top-level named constant (e.g., LICENSE_TEAMS_VOIP_ENTERPRISE) and replace its direct usage in isVoipModuleAvailable() and any other usages such as in useMediaCallPermission.ts / useMediaCallPermission so all checks reference the new constant instead of the literal string; update exports/imports as needed to keep consistent naming with existing constants LICENSE_OMNICHANNEL_MOBILE_ENTERPRISE and LICENSE_LIVECHAT_ENTERPRISE.ios/Libraries/AppDelegate+Voip.swift (1)
51-62: Replace magicreason: 1with a namedCXCallEndedReasonvalue.
RNCallKeep.endCall(withUUID:reason:)accepts the raw value ofCXCallEndedReason;1corresponds to.failed. Using the named constant (or a file-local constant) avoids ambiguity for future readers and makes intent explicit — this placeholder is being ended because the VoIP payload was unparseable or expired, i.e., a failed call.♻️ Proposed refactor
+import CallKit + fileprivate let voipAppDelegateLogTag = "RocketChat.AppDelegate+Voip" @@ - RNCallKeep.endCall(withUUID: callUUID, reason: 1) + RNCallKeep.endCall(withUUID: callUUID, reason: Int32(CXCallEndedReason.failed.rawValue)) completion()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/AppDelegate`+Voip.swift around lines 51 - 62, The call uses a magic literal reason: 1 when ending the placeholder call in the reportPlaceholderCallAndEnd closure; replace that magic number with the named CXCallEndedReason value (e.g., use CXCallEndedReason.failed.rawValue or a file-local constant like let placeholderEndReason = CXCallEndedReason.failed.rawValue) and pass that to RNCallKeep.endCall(withUUID:callUUID, reason: placeholderEndReason) so the intent is explicit in AppDelegate+Voip.swift and the RNCallKeep.endCall(withUUID:reason:) call is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Libraries/AppDelegate`+Voip.swift:
- Around line 84-91: The happy-path currently calls completion() immediately
after invoking reportVoipIncomingCallToCallKit, which violates PushKit rules;
instead pass the completion closure into reportVoipIncomingCallToCallKit's
onReportComplete parameter (i.e. replace onReportComplete: {} and the subsequent
completion() call with onReportComplete: { completion() }) so the completion
handler runs only after reportNewIncomingCall's callback completes.
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 169-181: startCallByRoom currently calls the async startCall
fire-and-forget which can produce unhandled rejections; update startCallByRoom
to be async and await this.startCall(...) inside a try/catch (or at minimum
attach a .catch) so errors from this.instance?.startCall are observed—use the
same logger used elsewhere (e.g., console.error or the module's logger) to log
the error and optionally rethrow or propagate a failure signal instead of
silently dropping it; reference the startCallByRoom and startCall methods and
the getUidDirectMessage call to locate where to add the await/try-catch.
---
Nitpick comments:
In `@app/lib/methods/enterpriseModules.ts`:
- Around line 65-68: Extract the magic string 'teams-voip' into a top-level
named constant (e.g., LICENSE_TEAMS_VOIP_ENTERPRISE) and replace its direct
usage in isVoipModuleAvailable() and any other usages such as in
useMediaCallPermission.ts / useMediaCallPermission so all checks reference the
new constant instead of the literal string; update exports/imports as needed to
keep consistent naming with existing constants
LICENSE_OMNICHANNEL_MOBILE_ENTERPRISE and LICENSE_LIVECHAT_ENTERPRISE.
In `@ios/Libraries/AppDelegate`+Voip.swift:
- Around line 51-62: The call uses a magic literal reason: 1 when ending the
placeholder call in the reportPlaceholderCallAndEnd closure; replace that magic
number with the named CXCallEndedReason value (e.g., use
CXCallEndedReason.failed.rawValue or a file-local constant like let
placeholderEndReason = CXCallEndedReason.failed.rawValue) and pass that to
RNCallKeep.endCall(withUUID:callUUID, reason: placeholderEndReason) so the
intent is explicit in AppDelegate+Voip.swift and the
RNCallKeep.endCall(withUUID:reason:) call is clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6357a235-b425-450c-8676-a32bbf736daa
📒 Files selected for processing (6)
app/containers/NewMediaCall/CreateCall.tsxapp/lib/methods/enterpriseModules.test.tsapp/lib/methods/enterpriseModules.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaSessionInstance.tsios/Libraries/AppDelegate+Voip.swift
💤 Files with no reviewable changes (1)
- app/lib/services/restApi.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/methods/enterpriseModules.tsapp/containers/NewMediaCall/CreateCall.tsxapp/lib/methods/enterpriseModules.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/methods/enterpriseModules.tsapp/containers/NewMediaCall/CreateCall.tsxapp/lib/methods/enterpriseModules.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/methods/enterpriseModules.tsapp/containers/NewMediaCall/CreateCall.tsxapp/lib/methods/enterpriseModules.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/enterpriseModules.tsapp/containers/NewMediaCall/CreateCall.tsxapp/lib/methods/enterpriseModules.test.tsapp/lib/services/voip/MediaSessionInstance.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in app/containers/ directory
Files:
app/containers/NewMediaCall/CreateCall.tsx
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Files:
app/lib/services/voip/MediaSessionInstance.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
app/lib/methods/enterpriseModules.tsapp/containers/NewMediaCall/CreateCall.tsxapp/lib/methods/enterpriseModules.test.tsapp/lib/services/voip/MediaSessionInstance.tsios/Libraries/AppDelegate+Voip.swift
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to @(app/sagas/videoConf.ts|app/lib/methods/videoConf.ts) : Manage video conferencing via Redux actions/reducers/sagas in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using server-managed Jitsi integration; do not conflate with VoIP
Applied to files:
app/lib/methods/enterpriseModules.tsapp/containers/NewMediaCall/CreateCall.tsx
📚 Learning: 2026-04-07T17:49:25.836Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T17:49:25.836Z
Learning: Applies to **/*.{js,ts,jsx,tsx} : Use explicit error handling with try/catch blocks for async operations
Applied to files:
app/containers/NewMediaCall/CreateCall.tsx
🔇 Additional comments (2)
app/lib/methods/enterpriseModules.test.ts (1)
1-24: LGTM!Clear, focused unit coverage for both branches of
isVoipModuleAvailable(); store setup is properly isolated viabeforeEach->clearEnterpriseModules().app/containers/NewMediaCall/CreateCall.tsx (1)
18-30: LGTM — proper async error surfacing.
await,try/catch, dismiss-on-success, and localized fallback message are all in order and align with the newPromise<void>signature ofstartCall.One optional consideration:
e.messagecoming from the underlying media-signaling/WebRTC layer can be a raw technical string. If that matters for UX, consider always showingI18n.t('VoIP_Call_Issue')and logginge.messageseparately — but keeping as-is is fine if the PR intent is to surface the underlying reason.
| reportVoipIncomingCallToCallKit( | ||
| callUUID: callId, | ||
| handle: caller, | ||
| handleType: "generic", | ||
| hasVideo: false, | ||
| localizedCallerName: caller, | ||
| supportsHolding: true, | ||
| supportsDTMF: true, | ||
| supportsGrouping: false, | ||
| supportsUngrouping: false, | ||
| fromPushKit: true, | ||
| payload: payloadDict, | ||
| withCompletionHandler: {} | ||
| onReportComplete: {} | ||
| ) | ||
| completion() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On iOS 13+ PushKit didReceiveIncomingPushWith, must completion() be invoked only after CallKit reportNewIncomingCall's completion handler fires?
💡 Result:
Yes. For iOS 13.0+ VoIP PushKit callbacks, you should invoke the PushKit completion only after you have finished reporting the call to CallKit—i.e., inside the completion block of CXProvider.reportNewIncomingCall(with:update:completion:) (after it fires), not before. Apple’s PushKit delegate documentation explicitly notes that when calling CallKit from pushRegistry(_:didReceiveIncomingPushWith:for:completion:), “If you fail to report a call to CallKit, the system will terminate your app,” and the PushKit “Responding to VoIP Notifications” example calls PushKit completion from inside the CallKit reportNewIncomingCall completion block. This implies the required ordering: report to CallKit first, then tell PushKit you’re done after that report completes.
Citations:
- 1: https://developer.apple.com/documentation/pushkit/pkpushregistrydelegate/pushregistry(_:didreceiveincomingpushwith:for:completion:)
- 2: https://developer.apple.com/documentation/pushkit/responding-to-voip-notifications-from-pushkit
- 3: https://developer.apple.com/documentation/callkit/cxprovider/reportnewincomingcall(with:update:completion:)
🏁 Script executed:
wc -l ios/Libraries/AppDelegate+Voip.swiftRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 117
🏁 Script executed:
cat -n ios/Libraries/AppDelegate+Voip.swift | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4107
Happy-path calls completion() before CallKit reporting completes — violates Apple's PushKit requirement.
The error path correctly defers completion() into the onReportComplete callback (line 59), but the happy path passes an empty onReportComplete: {} and then calls completion() synchronously at line 91. Per Apple's PushKit documentation (iOS 13+), completion() must be invoked only after CallKit's reportNewIncomingCall callback fires. Calling it before can cause the system to terminate the app for not fulfilling the PushKit entitlement contract.
Route completion() through the helper's onReportComplete parameter to match the error path:
Proposed fix
reportVoipIncomingCallToCallKit(
callUUID: callId,
handle: caller,
localizedCallerName: caller,
payload: payloadDict,
- onReportComplete: {}
+ onReportComplete: { completion() }
)
- completion()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reportVoipIncomingCallToCallKit( | |
| callUUID: callId, | |
| handle: caller, | |
| handleType: "generic", | |
| hasVideo: false, | |
| localizedCallerName: caller, | |
| supportsHolding: true, | |
| supportsDTMF: true, | |
| supportsGrouping: false, | |
| supportsUngrouping: false, | |
| fromPushKit: true, | |
| payload: payloadDict, | |
| withCompletionHandler: {} | |
| onReportComplete: {} | |
| ) | |
| completion() | |
| reportVoipIncomingCallToCallKit( | |
| callUUID: callId, | |
| handle: caller, | |
| localizedCallerName: caller, | |
| payload: payloadDict, | |
| onReportComplete: { completion() } | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Libraries/AppDelegate`+Voip.swift around lines 84 - 91, The happy-path
currently calls completion() immediately after invoking
reportVoipIncomingCallToCallKit, which violates PushKit rules; instead pass the
completion closure into reportVoipIncomingCallToCallKit's onReportComplete
parameter (i.e. replace onReportComplete: {} and the subsequent completion()
call with onReportComplete: { completion() }) so the completion handler runs
only after reportNewIncomingCall's callback completes.
1 participant
Proposed changes
This change set lands the first slice of the VoIP hardening plan (pre–PR #6918 ship blockers). iOS PushKit now reports invalid or expired payloads to CallKit and ends the placeholder call immediately, which avoids entitlement issues when the payload cannot be honored. The
teams-voipenterprise module check is restored so VoIP stays behind licensing. Outbound calls from the new-media call sheet awaitstartCall, surface failures to the user, and only dismiss the action sheet on success.registerPushTokenno longer blocks APNs registration on iOS when the VoIP token is missing, while still sending both tokens together when VoIP is available. A small helper inAppDelegate+Voip.swiftremoves duplicated CallKit reporting. Unit tests coverisVoipModuleAvailable.Issue(s)
How to test or reproduce
teams-voip, confirm VoIP entry points respect licensing; withteams-voippresent, confirm VoIP remains available.push.tokenstill registers when APNs token exists; after VoIP token arrives, confirm a follow-up registration includes both fields.yarn test --testPathPattern='enterpriseModules.test|MediaSessionInstance.test'.Screenshots
N/A (behavioral and platform fixes).
Types of changes
Checklist
Further comments
Split from the VoIP review plan; targets
developas requested. No native project file edits beyond Swift inios/Libraries/.Summary by CodeRabbit